[ruff] Fix false positive on global keyword (RUF052)
#15235
Conversation
| @@ -1,4 +1,4 @@ | |||
| # Correct | |||
| ##################### Correct ##################### | |||
There was a problem hiding this comment.
This might be just personal taste, so happy to remove!
But when I saw just # Correct, I wasn't sure if that was demarcating a section of code. I personally find this more clear.
EDIT: should probably use blocker header format (otherwise conflicts with E266).
Garrett-R
force-pushed
the
garrett/14996/ruff052-fix-global-keyword
branch
from
ce679c7 to
70f2f89
Compare
| _num = 1 | ||
|
|
||
| def print_num(): | ||
| print(_num) |
There was a problem hiding this comment.
Note: I inserted the test here into the ############### Correct ############# section, seemed right thing to do.
But given it's not being at the end of this file, it means I got some large diffs in the fixture files. I reviewed the diffs and there's no "real" changes to them, as expected.
| @@ -37,6 +37,17 @@ def fun(): | |||
| _x = "reassigned global" | |||
| return _x | |||
|
|
|||
| # (we had a false positive when a global var is used like this in 2 functions) | |||
There was a problem hiding this comment.
(am I overcommenting? I was just worried that it might not be clear that this is a single "test" spread across 2 functions, but happy to remove)
|
| @@ -1498,7 +1498,7 @@ impl<'a> SemanticModel<'a> { | |||
| source: self.node_id, | |||
| context: self.execution_context(), | |||
| exceptions: self.exceptions(), | |||
| flags: BindingFlags::empty(), | |||
| flags: BindingFlags::GLOBAL, | |||
There was a problem hiding this comment.
I don't think this is the right fix. The binding pushed here is for the variable _x in the global scope. But _x isn't considered a global-binding in the global scope. It is only when used in a nested scope. For example, the checker explicitly skips over the global handling when in the global scope here
ruff/crates/ruff_linter/src/checkers/ast/mod.rs
Lines 657 to 677 in d4ee6ab
But I think you're on the right track. What I find suspicious (and changing it fixes the bug as well) is that scope is self.scope_id but we add the binding to self.global_scope_mut. That's why I think it should actually be ScopeId::global() instead. But I'd like a confirmation from @charliermarsh on this change
|
@charliermarsh would you mind taking a look at my comment so that we can move forward with this PR? |
Summary
Fixes #14996.
BTW, the issue correctly reports a false positive given that the docs say "Only local variables in function scopes are flagged by the rule".
Test Plan
I added the false positive to the fixture and verified it indeed caused a false positive before my fix was done.